Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable generated items for more auth methods #7513

Merged
merged 73 commits into from
Oct 17, 2019
Merged

Conversation

madalynrose
Copy link
Contributor

@madalynrose madalynrose commented Sep 25, 2019

Enable OpenApi for more Auth Methods

This PR enables OpenApi to fully manage the Userpass, Cert, Okta, and Radius auth methods within the UI.

In addition to enabling, disabling and configuring the auth method, this PR includes CRUD actions for:

  • Userpass users
  • Cert certificates
  • Okta groups & users
  • Radius users
  • LDAP groups & users

QA Scenarios

  • sensitive field comes through in openApiToAttrs (users' passwords, for example)
  • editType comes through in openApiToAttrs
  • test creating and editing userpass users
  • test creating & editing certs certificates
  • test creating & editing Okta groups
  • test creating & editing Okta users
  • test creating & editing Radius users
  • test creating & editing LDAP groups
  • test creating & editing LDAP users

sdk/framework/path.go Outdated Show resolved Hide resolved
sdk/framework/path.go Outdated Show resolved Hide resolved
Copy link
Contributor

@kalafut kalafut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Go changes are fine except for a couple minor items and one odd err change that need review.

Copy link

@johncowen johncowen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @noelledaley !

I did an initial code only pass here and there seems to be a lot going on! Most of the things I've noted are just style things for the moment at least. I'm thinking of firing vault up and taking a closer look at this a bit later on, will shout you offline.

John

builtin/credential/cert/path_certs.go Show resolved Hide resolved
builtin/credential/userpass/path_users.go Show resolved Hide resolved
@@ -3,7 +3,7 @@ import { tabsForAuthSection } from 'vault/helpers/tabs-for-auth-section';
export default Route.extend({
beforeModel() {
let { methodType, paths } = this.modelFor('vault.cluster.access.method');
paths = paths ? paths.navPaths.reduce((acc, cur) => acc.concat(cur.path), []) : null;
paths = paths ? paths.paths.filter(path => path.navigation === true) : null;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No biggie, but the style:

paths.paths.filter(path => path.navigation)

was used everywhere else. I think here I personally like the more explicit style (this made me double think when I saw the less explicit style up above), but I think the more important point is to maybe change this so the style is all the same, whichever one you prefer.

ui/app/services/path-help.js Outdated Show resolved Hide resolved
ui/app/services/path-help.js Outdated Show resolved Hide resolved
ui/app/services/path-help.js Outdated Show resolved Hide resolved
ui/app/services/path-help.js Outdated Show resolved Hide resolved
Madalyn Parker and others added 19 commits October 9, 2019 16:58
…lt, change debugger to console.err in path-help, remove dynamic ui auth methods from tab count test
@andaley
Copy link
Contributor

andaley commented Oct 16, 2019

I've updated this PR to address all of the bugs I initially found. I removed the list of bugs and improvements and included them below for posterity. Should be good to go!

Bugfixes

  • access/userpass3/item/user is listing the users from v1/auth/userpass-1/users/?list=true until I refresh the page. This happens for LDAP groups and users as well. Appears to be a caching issue.
  • the tooltip on the certificate field when creating a certificate doesn't appear
  • links to generated items show page are wrong: http://localhost:4200/v1/auth/generated-user-okta/users/test-user 404 (Not Found)
  • editing an ldap group doesn't work. they are included in the request payload but aren't saved. instead, when you edit an existing group called "my-group", it creates a new group called "my-ldap/my-group". this happens for users as well.
  • deleting a nested item only works from the list page, not from the show page
  • handle the case where user names their group mygroup/foo/bar and tries to edit

Improvements

  • when you click on an auth method, it should link to the list page of the first generated item (the users or group page)
  • use new confirm component when disabling an auth method

@meirish
Copy link
Contributor

meirish commented Oct 16, 2019

When deleting an item from the show page, the flash message is incorrect - it says "undefined" instead of the item id.

@meirish
Copy link
Contributor

meirish commented Oct 16, 2019

We might want design to give this a once over, I'm not sure about the delete button in the header and the create button in the toolbar having a > instead of a +. Other than that minor flash message issue, the functionality looks good!

meirish
meirish previously approved these changes Oct 16, 2019
Copy link
Contributor

@meirish meirish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@noelledaley Mentioned a couple of small things, but the could easily be addressed later, so I'll let you make the call when to merge. 👍

@joshuaogle joshuaogle self-requested a review October 17, 2019 21:29
Copy link
Contributor

@joshuaogle joshuaogle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! This is going to be such a nice start for these auth methods

@andaley andaley merged commit 8f4530b into master Oct 17, 2019
@andaley andaley deleted the ui-generated-items branch October 17, 2019 23:19
catsby added a commit that referenced this pull request Oct 23, 2019
* master: (41 commits)
  changelog++
  cli: fix json output for namespace list command (#7705)
  Update CHANGELOG.md
  Vault Agent Template (#7652)
  hostutil: disable host info collection on openbsd (#7699)
  Shamir seals now come in two varieties: legacy and new-style.  (#7694)
  Update Title for AWS Marketplace (#7683)
  Bump go builder version
  Fix kv mod import and vendoring
  Fixing a typo with the sample payload (#7688)
  Update CHANGELOG.md
  Enable generated items for more auth methods (#7513)
  Update OIDC provider doc
  Update OIDC provider doc (#7693)
  Create .bundle and set group when running container (#7684)
  updates vendored api/client.go (#7692)
  Update circle config
  logical: remove unneeded error check in handleLogicalInternal (#7691)
  Update Go version
  Docs: add examples for when a seal rewrap is useful (#7689)
  ...
@@ -137,6 +165,10 @@ certificate.`,

HelpSynopsis: pathCertHelpSyn,
HelpDescription: pathCertHelpDesc,
DisplayAttrs: &framework.DisplayAttributes{
Action: "Create",
Copy link
Contributor

@andaley andaley Jul 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is what allows us dynamically generate the form for creating a "nested item" -- i.e., a certificate nested under the cert auth method.
image

},

"allowed_uri_sans": &framework.FieldSchema{
Type: framework.TypeCommaStringSlice,
Description: `A comma-separated list of URIs.
At least one must exist in the SANs. Supports globbing.`,
DisplayAttrs: &framework.DisplayAttributes{
Name: "Allowed URI SANs",
Group: "Constraints",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the Group specifies which field group the field should belong to. for example:

image

@@ -23,6 +23,10 @@ func pathListCerts(b *backend) *framework.Path {

HelpSynopsis: pathCertHelpSyn,
HelpDescription: pathCertHelpDesc,
DisplayAttrs: &framework.DisplayAttributes{
Navigation: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this boolean describes whether the item should have a tab in the navigation:

image


// EditType is the type of form field needed for a property
// e.g. "textarea" or "file"
EditType string `json:"editType,omitempty"`
Copy link
Contributor

@andaley andaley Jul 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are all of the available keys for the DisplayAttributes map, which tells the UI how to display the "thing" (i.e. certificate) or specific form field. see the full list here:

https://github.com/hashicorp/vault/blob/master/sdk/framework/path.go#L169-L196

}
helpUrl = `/v1/${apiPath}${path.slice(1)}?help=true`;

helpUrl = `/v1/${apiPath}${path.slice(1)}?help=true` || newModel.proto().getHelpUrl(backend);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is how we construct the URL for making the request to OpenAPI. if the ?help-true suffix ever changes for some reason you'll need to update it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants